Skip to content

fix(agent-runtime): release an unclosed think-tag open as text at flush - #1173

Open
nordicnode wants to merge 3 commits into
CodebuffAI:mainfrom
nordicnode:oss/think-tag-unclosed-open-1155
Open

fix(agent-runtime): release an unclosed think-tag open as text at flush#1173
nordicnode wants to merge 3 commits into
CodebuffAI:mainfrom
nordicnode:oss/think-tag-unclosed-open-1155

Conversation

@nordicnode

@nordicnode nordicnode commented Aug 31, 2026

Copy link
Copy Markdown

Summary

  • an explicit <think> open in the content stream no longer commits the rest of the step to the thinking box unconditionally. Resolution is armed from the conversation history, symmetric with the existing implicit-head machinery:
    • Clean lane — the last assistant turn never left an open unclosed (every model that pairs its tags): rule 1 stays free, paired blocks stream live per-delta with zero buffering. A genuine R1-style trace no longer waits for its close — measured by the streams a long well-formed trace per-delta on a clean lane, never buffered test.
    • Proven lanehistoryHasUnclosedOpen finds an open with no close after it in the last assistant turn: holdExplicitOpens holds from the open, a close settles it as reasoning, and the shared IMPLICIT_OPEN_BUDGET_CHARS (4000) or the end of the step releases it as text, so an answer behind a quoted tag is delayed but never swallowed (Harness: prompt-error orphans the run, unclosed think tag hides the answer, mid-stream 5xx/429 fatal while severed connections recover #1155, bug 2).
  • historyHasUnclosedOpen reads the last assistant message only, so the arm self-heals: a broken-template lane stays armed step after step, while a one-off quoted open arms exactly the next step and clears as soon as a properly paired reply lands — a session-wide scan would hold every later genuine trace because the user once asked about the tag.
  • a native reasoning_content chunk still disarms any hold and releases it as text.

This addresses the answer-swallowing part of #1155: whenever a model emits the tag as prose — writing docs, quoting a template, or a lane whose chat template is broken — the visible answer landed in the thinking box and the user saw an empty or short reply. History still stores the raw text, so this is a display/reclassification fix, not context loss.

Tradeoffs: the first step on a lane that starts leaving opens unclosed has no history to arm from (same accepted gap implicitOpen has; it self-heals on the next step). On an armed lane, a genuine short trace held under budget delays its display until the close arrives, bounded by the 4000-char budget. The review's "chunked flush of confirmed-safe prefixes" is not implementable — after an open, no prefix is confirmable until </think> or EOF (reasoning_delta has no retraction) — so the paired-vs-prose bet is made once, with history evidence, instead of per-chunk guesses.

Validation

  • bun test packages/agent-runtime/src/util/__tests__/think-tag-stream.test.ts — 33 pass, 0 fail (24 before: +5 historyHasUnclosedOpen, +2 clean-lane streaming, armed hold/budget/disarm tests reshaped)
  • package suite failure set byte-identical to the main baseline (2 pre-existing prompts-schema-handling fails + 2 agents-graveyard errors — stash-diff proved, timings excluded)
  • tsc --noEmit -p . clean for agent-runtime modulo those pre-existing errors
  • bunx prettier --check clean on all three changed files

Refs #1155


Replaces #1161 — auto-closed when the repo history was force-pushed (the original commits no longer exist in the new history). Rebased onto the new main per the maintainer's request; picking the review up here.

An explicit <think> open committed the rest of the step to the thinking
box unconditionally, so when a model wrote the tag as prose (docs,
quoted templates, a broken chat template) the visible answer landed in
the thinking box and the user saw an empty or short reply.

Give the explicit open the same treatment the implicit head already
has: hold until a close settles the block as reasoning, and release the
hold as text when the step ends without one — an answer is delayed,
never swallowed. The budget stays implicit-head-only: a genuine think
block can legitimately run long, and only its close (or flush) settles
it.

Refs CodebuffAI#1155
@codebuff-team

Copy link
Copy Markdown
Contributor

The instinct here is right: an unclosed <think> that's actually prose (docs, a broken chat template) shouldn't silently eat the rest of the answer, and the test for Write <think> like this in your docs (think-tag-stream.test.ts) demonstrates the bug well.

The concern is the fix for the common case. Before this change, an explicit <think>...</think> streamed its contents as reasoning segments in real time — that's what the docstring calls "unambiguous, free, always on." After this change, holdingForOpen is armed on every open tag, and per the new comment in addReasoning an explicit open gets no budget at all: "a genuine think block can legitimately run long, and only its close … settles it." That means for a model that legitimately emits a long <think> block — DeepSeek-R1-style reasoning, which can run to thousands of tokens — none of it reaches the client until the closing tag shows up. The new test holds an explicit open only until its close arrives confirms this is intentional, not an oversight, but it only exercises a two-chunk case; there's no test (or discussion) of the cost for a long, well-formed reasoning trace, which is presumably the majority case for models that use this tag at all.

That's a real UX/latency tradeoff: trading "swallowed answer on malformed input" for "no live reasoning display on well-formed input." It may be the right tradeoff, but the PR doesn't acknowledge it or measure it, and a maintainer will want to see how this behaves against an actual long-running reasoning model before porting it, or a bound on the hold (chunked flush of confirmed-safe prefixes, say) rather than all-or-nothing at close/flush.

Worth resolving before this goes in: either justify why buffering is acceptable for the paired-tag case, or find a way to keep streaming reasoning live while still handling the never-closes tail correctly.

@codebuff-team codebuff-team added bot:triaged Classified by the community triage bot pr:needs-work Right idea, not mergeable as written labels Sep 1, 2026
…_OPEN_HOLD_CHARS so paired traces stream live (review feedback on CodebuffAI#1173)
@nordicnode

Copy link
Copy Markdown
Author

The concern is right — holdingForOpen armed on every open tag with no bound meant paired traces buffered until close. Fixed in 9284bbe: the hold is now bounded, and past the bound the paired-block reading wins and the rest streams live.

  • New EXPLICIT_OPEN_HOLD_CHARS (= 4000, the implicit-head budget's magnitude, ≈ 1k tokens ≈ seconds at stream speed). An explicit open holds at most that much.
  • Past the bound the hold commits as reasoning (reuses confirmOpenHold) and subsequent deltas emit per-chunk, exactly as on main. A well-formed R1-style trace now costs only the first ~4000 chars of latency, not the whole trace; a later close still settles cleanly (the commit already marked the hold decided).
  • The never-closes tail keeps both resolutions: under the bound → released as text at flush (prose case unchanged, answer delayed but never swallowed); past the bound (already committed) → the trailing partial close flushes as reasoning, so a truncated close never leaks a stray </thi into the answer.

Honest residual: a prose answer that quotes the tag and runs past 4000 chars before the step ends gets its head committed to the thinking box. That is the same input class the paired-prose case (Write <think>…</think> with both tags quoted) always was — indistinguishable from a genuine block until the close, which never comes — and strictly no worse than main. The constant is the knob: raise it toward infinity to recover full buffering, lower it for latency.

Tests added for the long-trace shape the review asked for: commits the hold as reasoning past the bound and streams the rest live asserts per-delta emission after the bound, plus committed-flush-as-reasoning and implicit-head-still-releases-as-text.

Validation: bun test packages/agent-runtime/src/util/__tests__/think-tag-stream.test.ts — 27 pass, 0 fail (24 before). Package suite failure set identical to the main baseline (2 prompts-schema-handling + 2 agents-graveyard, both pre-existing). tsc --noEmit -p . clean for agent-runtime modulo those pre-existing errors. bunx prettier --check clean on both changed files.

Clean lanes (every model that pairs its tags) stream rule-1 blocks live with
zero buffering — the review's latency concern. Lanes whose last assistant turn
left an open unclosed arm holdExplicitOpens, so a prose-quoted tag delays the
answer instead of swallowing it, released as text at the shared
IMPLICIT_OPEN_BUDGET_CHARS or at flush. Replaces the commit-past-bound design
from 9284bbe, which re-swallowed long prose answers past the bound.
@nordicnode

nordicnode commented Sep 1, 2026

Copy link
Copy Markdown
Author

Follow-up in 1877b55, superseding the bounded-hold approach from 9284bbe: the bound traded one swallow for another (a prose answer longer than the bound after a quoted open would still commit its head to the thinking box — the exact #1155 class). The redesign keeps your latency case at literally zero cost and fixes the swallow only where it has actually been observed:

  • New historyHasUnclosedOpen scan: the LAST assistant turn contains an <think> with no </think> after it → arm holdExplicitOpens for this step. Symmetric with implicitOpen, armed from the same history for the same reason (lane property, not model property). A lane that pairs its tags — every well-formed R1-style trace, your majority case — never arms, so rule 1 stays free: paired blocks stream live per-delta, nothing buffered. Measured by streams a long well-formed trace per-delta on a clean lane, never buffered (5000 chars out on the first push).
  • On an armed lane: hold from the open, close settles as reasoning, past the shared IMPLICIT_OPEN_BUDGET_CHARS (4000) release as text — same give-up direction as the implicit head, since past that budget the step is answering, not thinking. No commit-as-reasoning anywhere, so no long-prose swallow.
  • First step on a genuinely broken lane (no history yet): one step of lag/swallow, then self-heals — the step itself leaves the unclosed open in history, which arms the next step. Same accepted gap implicitOpen already has.
  • Real residual on armed lanes: a genuine short trace held under budget delays its display until the close arrives (bounded by the 4000-char budget). Clean lanes — the majority — are untouched.

Why not the review's "chunked flush of confirmed-safe prefixes": after an open, no prefix is confirmable as text until </think> or EOF — reasoning_delta has no retraction — so any pre-close emission is a bet on the paired reading. That bet is exactly what the arming gate now decides with history evidence instead of guessing.

Validation: bun test packages/agent-runtime/src/util/__tests__/think-tag-stream.test.ts — 33 pass, 0 fail (9 new: history scan ×5, clean-lane live streaming ×2, armed hold/budget-release/native-disarm ×3 reshaped). Package suite failure set byte-identical to the main baseline (2 prompts-schema-handling fails, pre-existing — stash-diff proved). tsc --noEmit -p . clean for agent-runtime modulo those. bunx prettier --check clean on all three changed files.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot:triaged Classified by the community triage bot pr:needs-work Right idea, not mergeable as written

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants